feat(cors): Global CORS Configuration for AMRIT API Services#60
feat(cors): Global CORS Configuration for AMRIT API Services#60vishwab1 merged 4 commits intoPSMRI:developfrom
Conversation
WalkthroughThis pull request introduces a new CORS configuration approach. It adds the property Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DynamicCorsFilter
participant CorsConfig
participant Controller
Client->>DynamicCorsFilter: Send HTTP request with Origin header
DynamicCorsFilter->>DynamicCorsFilter: Check if Origin is allowed
alt Origin allowed
DynamicCorsFilter->>Client: Set Access-Control-Allow-Origin header
alt OPTIONS preflight request
DynamicCorsFilter->>Client: Respond with 200 OK (no further processing)
else Other HTTP methods
DynamicCorsFilter->>CorsConfig: Forward request
CorsConfig->>Controller: Apply CORS settings and forward request
Controller-->>Client: Return response
end
else Origin not allowed
DynamicCorsFilter->>CorsConfig: Forward request without CORS headers
CorsConfig->>Controller: Forward request
Controller-->>Client: Return response
end
Possibly related issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/config/CorsConfig.java (1)
20-20: Consider documenting the exposed headersThe explicit exposure of "Authorization" and "Jwttoken" headers is important for authentication and security purposes. Consider adding a brief comment explaining why these specific headers need to be exposed.
- .exposedHeaders("Authorization", "Jwttoken") // Explicitly expose headers if needed + .exposedHeaders("Authorization", "Jwttoken") // Exposing auth headers needed for client authentication
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/main/environment/ecd_ci.properties(1 hunks)src/main/environment/ecd_example.properties(1 hunks)src/main/java/com/iemr/ecd/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/ecd/controller/associate/AutoPreviewDialingController.java(1 hunks)src/main/java/com/iemr/ecd/controller/associate/BeneficiaryCallHistoryController.java(1 hunks)src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.java(2 hunks)src/main/java/com/iemr/ecd/controller/associate/CallClosureController.java(1 hunks)src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java(0 hunks)src/main/java/com/iemr/ecd/controller/callallocation/CallConfigurationController.java(0 hunks)src/main/java/com/iemr/ecd/controller/dataupload/DataTemplateController.java(0 hunks)src/main/java/com/iemr/ecd/controller/dataupload/DataUploadController.java(0 hunks)src/main/java/com/iemr/ecd/controller/masters/MastersController.java(0 hunks)src/main/java/com/iemr/ecd/controller/outboundworklist/CallStatisticsController.java(0 hunks)src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/AgentQualityAuditorMappingController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/ChartsController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/GradeConfigurationController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/QualityAuditController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/QualityAuditQuestionConfigurationController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/QualityAuditSectionConfigurationController.java(0 hunks)src/main/java/com/iemr/ecd/controller/quality/SampleSelectionConfigurationController.java(0 hunks)src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java(0 hunks)src/main/java/com/iemr/ecd/controller/reports/ReportController.java(21 hunks)
💤 Files with no reviewable changes (15)
- src/main/java/com/iemr/ecd/controller/masters/MastersController.java
- src/main/java/com/iemr/ecd/controller/callallocation/CallConfigurationController.java
- src/main/java/com/iemr/ecd/controller/questionare/EcdQuestionareController.java
- src/main/java/com/iemr/ecd/controller/quality/ChartsController.java
- src/main/java/com/iemr/ecd/controller/outboundworklist/OutBoundWorklistController.java
- src/main/java/com/iemr/ecd/controller/quality/QualityAuditQuestionConfigurationController.java
- src/main/java/com/iemr/ecd/controller/outboundworklist/CallStatisticsController.java
- src/main/java/com/iemr/ecd/controller/callallocation/CallAllocationController.java
- src/main/java/com/iemr/ecd/controller/quality/GradeConfigurationController.java
- src/main/java/com/iemr/ecd/controller/quality/SampleSelectionConfigurationController.java
- src/main/java/com/iemr/ecd/controller/dataupload/DataUploadController.java
- src/main/java/com/iemr/ecd/controller/dataupload/DataTemplateController.java
- src/main/java/com/iemr/ecd/controller/quality/AgentQualityAuditorMappingController.java
- src/main/java/com/iemr/ecd/controller/quality/QualityAuditController.java
- src/main/java/com/iemr/ecd/controller/quality/QualityAuditSectionConfigurationController.java
🔇 Additional comments (10)
src/main/environment/ecd_ci.properties (1)
31-31: Verify CORS configuration for CI environmentThe
cors.allowed-originsproperty is set to an empty value, which may prevent cross-origin requests in the CI environment. This could block frontend applications from accessing the API.Consider defining appropriate origins or using wildcard patterns based on your CI environment requirements. If the API is not intended to receive cross-origin requests in CI, this is acceptable.
src/main/environment/ecd_example.properties (1)
20-20: LGTM! Appropriate CORS configuration for local development.The configuration allows requests from any port on localhost and 127.0.0.1, which is suitable for local development environments.
src/main/java/com/iemr/ecd/controller/associate/BeneficiaryCallHistoryController.java (1)
46-46: LGTM! Removed CrossOrigin annotation for global configuration.Removing the
@CrossOrigin()annotation from this controller is appropriate when moving to a centralized CORS configuration approach.src/main/java/com/iemr/ecd/controller/reports/ReportController.java (1)
88-88: LGTM! Removed CrossOrigin annotations for global configuration.Removing all
@CrossOriginannotations from these report endpoints is appropriate when moving to a centralized CORS configuration approach.Also applies to: 112-112, 137-137, 161-161, 186-186, 211-211, 236-236, 261-261, 286-286, 311-311, 337-337, 362-362, 387-387, 412-412, 437-437, 462-462, 487-487, 512-512, 537-537, 562-562, 587-587
src/main/java/com/iemr/ecd/controller/associate/CallClosureController.java (1)
46-46: Removal of @crossorigin aligns with centralized CORS strategyThe @crossorigin annotation has been removed as part of migrating to a centralized CORS configuration approach.
src/main/java/com/iemr/ecd/controller/associate/AutoPreviewDialingController.java (1)
49-49: Removal of @crossorigin aligns with centralized CORS strategyThe @crossorigin annotation has been removed as part of migrating to a centralized CORS configuration approach.
src/main/java/com/iemr/ecd/config/CorsConfig.java (1)
1-25: Good implementation of centralized CORS configurationThe new CorsConfig class provides a centralized approach to CORS management with configurable origins through application properties. The configuration is comprehensive, covering all necessary CORS aspects.
src/main/java/com/iemr/ecd/controller/associate/BeneficiaryRegistrationController.java (3)
48-48: Removal of @crossorigin aligns with centralized CORS strategyThe @crossorigin annotation has been removed as part of migrating to a centralized CORS configuration approach.
59-59: Minor formatting improvementThe alignment of the @RequestHeader parameter has been improved for better readability.
65-65: Removal of @crossorigin aligns with centralized CORS strategyThe @crossorigin annotation has been removed as part of migrating to a centralized CORS configuration approach.
kevalkanp1011
changed the title
| springdoc.api-docs.enabled=false | ||
| springdoc.swagger-ui.enabled=false | ||
|
|
||
| cors.allowed-origins= |
There was a problem hiding this comment.
Can you put the value as @CORS_ALLOWED_ORIGINS@
value assign:- @CORS_ALLOWED_ORIGINS@
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/com/iemr/ecd/utils/DynamicCorsFilter.java (1)
26-29: Add error handling and optimize origin checking for better performance.The current implementation doesn't handle invalid origins explicitly and performs a list conversion on each request which may impact performance for high traffic applications.
Consider these improvements:
- Initialize a HashSet in a @PostConstruct method for faster lookups
- Add logging for rejected origins to aid debugging
@Component public class DynamicCorsFilter extends OncePerRequestFilter { @Value("${cors.allowed-origins}") private String[] allowedOrigins; + + private Set<String> allowedOriginsSet; + + @PostConstruct + public void init() { + allowedOriginsSet = new HashSet<>(Arrays.asList(allowedOrigins)); + } @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { String origin = request.getHeader("Origin"); - if (origin != null && Arrays.asList(allowedOrigins).contains(origin)) { + if (origin != null && allowedOriginsSet.contains(origin)) { response.setHeader("Access-Control-Allow-Origin", origin); + } else if (origin != null) { + logger.debug("Rejected CORS request from origin: " + origin); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/environment/ecd_example.properties(1 hunks)src/main/java/com/iemr/ecd/config/CorsConfig.java(1 hunks)src/main/java/com/iemr/ecd/utils/DynamicCorsFilter.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/environment/ecd_example.properties
- src/main/java/com/iemr/ecd/config/CorsConfig.java
🔇 Additional comments (2)
src/main/java/com/iemr/ecd/utils/DynamicCorsFilter.java (2)
1-13: Appropriate import statements and package declaration for CORS filter functionality.The import statements correctly include necessary classes for HTTP servlet handling, Spring annotations, and Java utilities needed by the filter.
14-19: Good approach using Spring Component and Value injection for CORS configuration.The class is correctly annotated as a Spring
@Componentand extendsOncePerRequestFilterwhich ensures the filter is applied only once per request. Using@Valueto inject allowed origins from configuration is a good practice for externalized configuration.
| package com.iemr.ecd.utils; | ||
|
|
||
| import jakarta.servlet.FilterChain; | ||
| import jakarta.servlet.ServletException; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.stereotype.Component; | ||
| import org.springframework.web.filter.OncePerRequestFilter; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Arrays; | ||
|
|
||
| @Component | ||
| public class DynamicCorsFilter extends OncePerRequestFilter { | ||
|
|
||
| @Value("${cors.allowed-origins}") | ||
| private String[] allowedOrigins; | ||
|
|
||
| @Override | ||
| protected void doFilterInternal(HttpServletRequest request, | ||
| HttpServletResponse response, | ||
| FilterChain filterChain) | ||
| throws ServletException, IOException { | ||
|
|
||
| String origin = request.getHeader("Origin"); | ||
| if (origin != null && Arrays.asList(allowedOrigins).contains(origin)) { | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| } | ||
|
|
||
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | ||
| response.setStatus(HttpServletResponse.SC_OK); | ||
| } else { | ||
| filterChain.doFilter(request, response); | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a unit test to verify CORS behavior.
This filter contains critical security-related logic for handling cross-origin requests. Adding comprehensive unit tests would ensure it functions correctly across different scenarios.
Consider adding tests that verify:
- Allowed origins are properly accepted
- Disallowed origins are properly rejected
- Preflight requests are handled correctly
- Non-preflight requests proceed through the filter chain
Would you like me to propose a unit test implementation for this class?
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | ||
| response.setStatus(HttpServletResponse.SC_OK); | ||
| } else { | ||
| filterChain.doFilter(request, response); | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete preflight request handling.
For OPTIONS requests (preflight), the filter returns a 200 OK status but doesn't set the required CORS response headers that browsers expect in preflight responses (Access-Control-Allow-Methods, Access-Control-Allow-Headers, etc.). This could cause preflight requests to fail.
Consider updating the implementation to include all necessary CORS headers for preflight requests:
if ("OPTIONS".equalsIgnoreCase(request.getMethod())) {
response.setStatus(HttpServletResponse.SC_OK);
+ response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS");
+ response.setHeader("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept");
+ response.setHeader("Access-Control-Max-Age", "3600");
+ response.setHeader("Access-Control-Allow-Credentials", "true");
} else {
filterChain.doFilter(request, response);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | |
| response.setStatus(HttpServletResponse.SC_OK); | |
| } else { | |
| filterChain.doFilter(request, response); | |
| } | |
| } | |
| if ("OPTIONS".equalsIgnoreCase(request.getMethod())) { | |
| response.setStatus(HttpServletResponse.SC_OK); | |
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS"); | |
| response.setHeader("Access-Control-Allow-Headers", "Authorization, Content-Type, Accept"); | |
| response.setHeader("Access-Control-Max-Age", "3600"); | |
| response.setHeader("Access-Control-Allow-Credentials", "true"); | |
| } else { | |
| filterChain.doFilter(request, response); | |
| } |
| @Override | ||
| protected void doFilterInternal(HttpServletRequest request, | ||
| HttpServletResponse response, | ||
| FilterChain filterChain) | ||
| throws ServletException, IOException { | ||
|
|
||
| String origin = request.getHeader("Origin"); | ||
| if (origin != null && Arrays.asList(allowedOrigins).contains(origin)) { | ||
| response.setHeader("Access-Control-Allow-Origin", origin); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing additional CORS headers in the response.
While the filter correctly sets the Access-Control-Allow-Origin header based on allowed origins, it doesn't set other important CORS headers like Access-Control-Allow-Methods, Access-Control-Allow-Headers, and Access-Control-Allow-Credentials. This might be intentional if these are set in the CorsConfig class mentioned in the PR summary, but worth verifying.
🏁 Script executed:
#!/bin/bash
# Check if there are other places in the codebase where CORS headers are set
rg -A 5 "Access-Control-Allow" --type javaLength of output: 1183
🏁 Script executed:
#!/bin/bash
# Locate and display the CorsConfig class to verify if it sets other CORS headers
rg -n "class CorsConfig" --type java
rg -A10 "class CorsConfig" --type javaLength of output: 1130
Consolidate CORS settings and include credentials
We verified that CorsConfig (src/main/java/com/iemr/ecd/config/CorsConfig.java) already registers allowed origins, methods, and headers via
registry.addMapping("/**")
.allowedOrigins(allowedOrigins)
.allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS")
.allowedHeaders("*");but it omits allowCredentials(true). At the same time, DynamicCorsFilter (src/main/java/com/iemr/ecd/utils/DynamicCorsFilter.java) only sets Access-Control-Allow-Origin (and status for OPTIONS) and will override any other headers set by Spring’s CORS support. To avoid inconsistent or missing CORS response headers, please:
- Add
.allowCredentials(true)to theCorsConfigmapping. - Either remove the custom filter and rely solely on
CorsConfig, or updateDynamicCorsFilter#doFilterInternalto also set:Access-Control-Allow-Methods: GET,POST,PUT,DELETE,OPTIONSAccess-Control-Allow-Headers: <value>(e.g.*or echoAccess-Control-Request-Headers)Access-Control-Allow-Credentials: true
Example diffs:
--- a/src/main/java/com/iemr/ecd/config/CorsConfig.java
@@ public void addCorsMappings(CorsRegistry registry) {
- .allowedHeaders("*");
+ .allowedHeaders("*")
+ .allowCredentials(true);--- a/src/main/java/com/iemr/ecd/utils/DynamicCorsFilter.java
@@ doFilterInternal(...){
if (origin != null && Arrays.asList(allowedOrigins).contains(origin)) {
response.setHeader("Access-Control-Allow-Origin", origin);
+ response.setHeader("Access-Control-Allow-Methods", "GET,POST,PUT,DELETE,OPTIONS");
+ response.setHeader("Access-Control-Allow-Headers", request.getHeader("Access-Control-Request-Headers"));
+ response.setHeader("Access-Control-Allow-Credentials", "true");
}
3 participants
📋 Description
JIRA ID:
This PR introduces a global CORS configuration for the AMRIT platform API services, in alignment with requirements. The goal is to enhance cross-origin request handling by removing controller-level CORS annotations and setting up centralized, environment-based CORS policies.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Chores